-
Notifications
You must be signed in to change notification settings - Fork 121
[Analytics Hub] Exit Analytics Hub and display an error if date range selection fails #8298
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
You can test the changes from this Pull Request by:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It works well in my testing!
Just a few small considerations/ideas for improvement:
- In the
trunkwe have additional refresh action which error may be unhandled. - Do you think we can connect dismiss + notice actions directly, so it's more obvious without relying on UIKit lifecycle (viewWillDisappear)? For example:
dismiss(with notice: Notice?)on HostingVC can trigger both actions with clear intent. errorSelectingTimeRangefeels like duplicated information when we already have notice set and error thrown. Can we trigger notice+dismiss in thecatchclause?
| super.viewWillDisappear(animated) | ||
|
|
||
| // Show any notice that should have been presented before the underlying disappears. | ||
| enqueuePendingNotice(rootView.viewModel.notice, using: systemNoticePresenter) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Xcode shows a warning here for each navigation:
Accessing StateObject's object without being installed on a View. This will create a new instance each time.
|
Thanks for the review @ealeksandrov! I took a look at your suggestions; let me know what you think.
Thanks for the heads up! I merged
I like the idea of making the intent clearer like this, but if we don't rely on the UIKit lifecycle it will change the timing of the notice. Using Simulator.Screen.Recording.-.iPhone.14.Pro.-.2022-12-05.at.15.32.38.mp4I prefer the timing that it has now, but I've updated the dismiss action in 7aa749f so it's more clearly connected to the notice. (This also resolves the Xcode warning you observed.) This way it still uses the UIKit lifecycle for the notice timing, but it also makes it clearer that we're both dismissing the view and handling the notice.
That's reasonable. It feels a little odd to me to call UI code directly in the view model, so I've updated the catch clause in 228f144 to just set the notice (removing |
ealeksandrov
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for all changes and the demo, I agree that current implementation looks better!
One last nit: can we rename notice property and its comment to describe that setting it will dismiss the view? Something like "dismissNotice"?
Part of: #8213
Description
When selecting a date range fails, we now pop out of the Analytics Hub back to the My Store view and display a notice about the error.
We expect this should only happen in rare cases, when
AnalyticsHubTimeRangeSelectionfails to unwrap the current and previous time ranges.Changes
AnalyticsHubTimeRangeSelection.TimeRangeGeneratorErrorerrors. We set an error notice and seterrorSelectingTimeRangeto true.errorSelectingTimeRangeis true.Testing
You can force a failed date range selection by temporarily editing one of the methods that unwraps the time range to always throw an error:
woocommerce-ios/WooCommerce/Classes/ViewRelated/Dashboard/Analytics Hub/Time Range/AnalyticsHubTimeRangeSelection.swift
Lines 68 to 73 in 0af536a
Then:
Screenshots
DateRangeSelectionError.mp4
Submitter Checklist
Update release notes:
RELEASE-NOTES.txtif necessary.